Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: Replace robin-hood-hashing with unordered_dense #7558

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

build: Replace robin-hood-hashing with unordered_dense #7558

wants to merge 1 commit into from

Conversation

arno-lunarg
Copy link
Contributor

@arno-lunarg arno-lunarg commented Feb 22, 2024 •
edited
Loading

Follow up to #7473

Code is not compiling when using ankerl::unordered_dense::map, but does when using ankerl::unordered_dense::segmented_map (locally, on windows), a version that better mitigates memory allocation made when adding new elements. This is also a goal, next step is to measure if it indeed improves our memory footprint when initializing static maps.

@arno-lunarg arno-lunarg requested a review from a team as a code owner February 22, 2024 10:42
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 137774.

@arno-lunarg arno-lunarg marked this pull request as draft February 22, 2024 10:43
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15859 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 137784.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15860 running.

spencer-lunarg
spencer-lunarg reviewed Feb 22, 2024
View reviewed changes
layers/state_tracker/image_state.cpp
@@ -667,7 +667,7 @@ std::vector<VkPresentModeKHR> Surface::GetPresentModes(VkPhysicalDevice phys_dev
assert(phys_dev);
std::vector<VkPresentModeKHR> result;
if (auto search = present_modes_data_.find(phys_dev); search != present_modes_data_.end()) {
for (auto mode = search->second.begin(); mode != search->second.end(); mode++) {
for (auto mode = search->second.begin(); mode != search->second.end(); ++mode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremyg-lunarg so this is the type of issue failing the build before 😆

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15860 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 138426.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15875 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 138471.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15877 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 138518.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15878 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15878 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 139599.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15895 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 139628.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15897 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 15897 passed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 144130.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16004 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16004 passed.

jeremyg-lunarg
jeremyg-lunarg reviewed Mar 4, 2024
View reviewed changes
tests/unit/shader_interface.cpp Outdated Show resolved Hide resolved
layers/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/CMakeLists.txt Outdated Show resolved Hide resolved
layers/CMakeLists.txt
if (USE_ROBIN_HOOD_HASHING)
target_link_libraries(VkLayer_utils PUBLIC robin_hood::robin_hood)
target_compile_definitions(VkLayer_utils PUBLIC USE_ROBIN_HOOD_HASHING)
find_package(unordered_dense CONFIG)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching from USE_ROBIN_...* to USE_UNORDERED_* is going to require changes in Internal CI and the SDK build stuff. I wonder if we should do something like USE_CUSTOM_HASHMAP instead in case we ever decide to change implementations again. Curious what others think about this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea I think. I only used this name in replacement of the C++ define, explicit mentions of unordered dense are still there in files like vvl.yml

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 145333.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16031 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16031 passed.

@artem-lunarg
Copy link
Contributor

artem-lunarg commented Mar 6, 2024 •
edited
Loading

From what I understand original robin-hood library implements drop-in replacement of std::unordered{map/set}. The new library ignores std iterator validity requirements, for performance reasons (please correct me early, since I did not dig deep into this). My question how we should support switching between std and custom library? With robin-hood it was easy because the interfaces were compatible. With new library it's possible to write code that won't work correctly with std (but you might want it because that's the reason why new lib is faster). Is it a valid concern? Are there use cases that require hash container from a standard stl implementation, so we need to support both options?

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 149495.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16107 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16107 passed.

@arno-lunarg arno-lunarg marked this pull request as ready for review March 13, 2024 09:04
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 150694.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16131 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16131 passed.

@filnet
Copy link
Contributor

filnet commented Mar 16, 2024

Would it make sense to error out (or emit some warning) if USE_ROBIN_HOOD_HASHING is set ?
If not some projects that build VVL might not realize they silently switched to the default map.

For example: https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-vulkan-validation-layers/PKGBUILD#L68

@spencer-lunarg
Copy link
Contributor

Would it make sense to error out (or emit some warning) if USE_ROBIN_HOOD_HASHING is set ?

I agree, @arno-lunarg just add

if (USE_ROBIN_HOOD_HASHING)
  message(WARNING "USE_ROBIN_HOOD_HASHING was removed, please use USE_CUSTOM_HASH_MAP now .")
endif ()

@filnet
Copy link
Contributor

filnet commented Mar 17, 2024

nit: there is an extraneous space before the ending colon.

if (USE_ROBIN_HOOD_HASHING)
  message(WARNING "USE_ROBIN_HOOD_HASHING was removed, please use USE_CUSTOM_HASH_MAP now .")
endif ()

@jeremyg-lunarg @arno-lunarg
build: Replace robin-hood-hashing with unordered_dense
90b6179 
robin-hood-hashing is no longer developed. unordered_dense is its
replacement, by the same author.
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 151977.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16141 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16141 aborted.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 152168.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16150 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16150 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 152212.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16153 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 152239.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16155 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 16155 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spencer-lunarg spencer-lunarg spencer-lunarg left review comments

@jeremyg-lunarg jeremyg-lunarg jeremyg-lunarg requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@arno-lunarg @ci-tester-lunarg @spencer-lunarg @artem-lunarg @filnet @jeremyg-lunarg